fix: plugin_loader retries with --ignore-installed on apt/pip RECORD conflicts#386
Conversation
…t package satisfies pin install_dependencies treated any "uninstall-no-record-file" pip failure as "dependency satisfied" and wrote the success marker without ever attempting --ignore-installed, unlike install_dependencies_apt.py and safe_pip_install.sh (added in #385 for the Plugin Store/first-time-install paths). A plugin pinning a newer version of a system-managed package (e.g. requests) would silently keep running against whatever version apt shipped, while the marker file claimed the pinned requirement was met. Now retries the same install with --ignore-installed on that specific failure so pip actually lays the pinned version down (shadowing the system-managed copy) before falling back to the prior tolerant behavior if the retry itself fails too. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X1NnDduw53kTe67i5zWwYx
|
Warning Review limit reached
Next review available in: 48 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughPluginLoader.install_dependencies now retries a failed pip install with --ignore-installed when stderr contains "uninstall-no-record-file", logging a warning before proceeding regardless of retry outcome. New tests verify retry invocation and both success and failure retry paths return True. ChangesApt conflict retry logic
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Same generic Bandit/semgrep pattern-match on non-literal subprocess.run argv flagged in #385's install_requirements_file, now on the new --ignore-installed retry call added here: list-form argv (no shell=True), sys.executable is this process's own interpreter, and requirements_file is built internally by find_plugin_directory, never raw external input. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X1NnDduw53kTe67i5zWwYx
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/plugin_system/plugin_loader.py (1)
256-276: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRetry timeout bypasses tolerant fallback.
If the retry
subprocess.runraisesTimeoutExpired, it propagates to the outer handler (line 283) and returnsFalse, causing plugin load to fail. This is inconsistent with the PR's stated behavior of tolerating all retry failures — a non-zero retry return code returnsTrue, but a retry timeout returnsFalse. Wrap the retry in its owntry/except subprocess.TimeoutExpiredso timeouts are treated the same as other retry failures.🛡️ Proposed fix: handle retry timeout locally
if "uninstall-no-record-file" in stderr: self.logger.warning( "Dependencies for %s conflict with a system-managed package " "(no pip RECORD); retrying with --ignore-installed: %s", plugin_id, stderr.strip() ) # sys.executable is this process's own interpreter (not # attacker-influenced), and requirements_file is a path built # internally by find_plugin_directory, never raw external input. - retry_result = subprocess.run( # nosec B603 - no shell invoked (list-form argv) # nosemgrep - [sys.executable, "-m", "pip", "install", "--break-system-packages", - "--ignore-installed", "-r", requirements_file], - capture_output=True, - text=True, - timeout=timeout, - check=False - ) - if retry_result.returncode != 0: - self.logger.warning( - "Retry with --ignore-installed also failed for %s; assuming the " - "system-managed version satisfies the requirement: %s", - plugin_id, (retry_result.stderr or "").strip() - ) + try: + retry_result = subprocess.run( # nosec B603 - no shell invoked (list-form argv) # nosemgrep + [sys.executable, "-m", "pip", "install", "--break-system-packages", + "--ignore-installed", "-r", requirements_file], + capture_output=True, + text=True, + timeout=timeout, + check=False + ) + if retry_result.returncode != 0: + self.logger.warning( + "Retry with --ignore-installed also failed for %s; assuming the " + "system-managed version satisfies the requirement: %s", + plugin_id, (retry_result.stderr or "").strip() + ) + except subprocess.TimeoutExpired: + self.logger.warning( + "Retry with --ignore-installed timed out for %s; assuming the " + "system-managed version satisfies the requirement", + plugin_id + ) try: with open(marker_file, 'w', encoding='utf-8') as fh: fh.write(current_hash) ensure_file_permissions(Path(marker_file), get_plugin_file_mode()) except OSError as marker_err: self.logger.debug("Could not write dependency marker for %s: %s", plugin_id, marker_err) return True🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plugin_system/plugin_loader.py` around lines 256 - 276, The retry path in plugin_loader’s dependency install logic is inconsistent because a subprocess timeout still escapes to the outer handler and fails plugin loading. Update the retry block around subprocess.run in the plugin_loader method that handles requirements_file installs to catch subprocess.TimeoutExpired locally, log it the same way as other retry failures, and continue with the tolerant fallback so the function still returns True when the retry cannot complete.
🧹 Nitpick comments (1)
src/plugin_system/plugin_loader.py (1)
238-276: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract marker-writing helper to eliminate duplication.
The marker-writing block at lines 270-275 is identical to lines 228-233. Extract a small helper so future changes to marker logic stay in sync across both the success and retry-fallback paths.
♻️ Proposed refactor: extract _write_dependency_marker helper
+ def _write_dependency_marker( + self, marker_file: str, current_hash: str, plugin_id: str + ) -> None: + """Write the dependency hash marker file.""" + try: + with open(marker_file, 'w', encoding='utf-8') as fh: + fh.write(current_hash) + ensure_file_permissions(Path(marker_file), get_plugin_file_mode()) + except OSError as marker_err: + self.logger.debug("Could not write dependency marker for %s: %s", plugin_id, marker_err) + def install_dependencies(Then replace both call sites:
if result.returncode == 0: - try: - with open(marker_file, 'w', encoding='utf-8') as fh: - fh.write(current_hash) - ensure_file_permissions(Path(marker_file), get_plugin_file_mode()) - except OSError as marker_err: - self.logger.debug("Could not write dependency marker for %s: %s", plugin_id, marker_err) + self._write_dependency_marker(marker_file, current_hash, plugin_id) self.logger.info("Dependencies installed successfully for %s", plugin_id) return Trueif retry_result.returncode != 0: self.logger.warning( "Retry with --ignore-installed also failed for %s; assuming the " "system-managed version satisfies the requirement: %s", plugin_id, (retry_result.stderr or "").strip() ) - try: - with open(marker_file, 'w', encoding='utf-8') as fh: - fh.write(current_hash) - ensure_file_permissions(Path(marker_file), get_plugin_file_mode()) - except OSError as marker_err: - self.logger.debug("Could not write dependency marker for %s: %s", plugin_id, marker_err) + self._write_dependency_marker(marker_file, current_hash, plugin_id) return True🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plugin_system/plugin_loader.py` around lines 238 - 276, The marker-writing logic in plugin_loader’s dependency install flow is duplicated between the normal success path and the uninstall-no-record-file retry fallback. Extract a small helper such as _write_dependency_marker in PluginLoader to handle writing current_hash to marker_file and calling ensure_file_permissions, then replace both existing blocks with calls to that helper so marker behavior stays consistent in both paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/plugin_system/plugin_loader.py`:
- Around line 256-276: The retry path in plugin_loader’s dependency install
logic is inconsistent because a subprocess timeout still escapes to the outer
handler and fails plugin loading. Update the retry block around subprocess.run
in the plugin_loader method that handles requirements_file installs to catch
subprocess.TimeoutExpired locally, log it the same way as other retry failures,
and continue with the tolerant fallback so the function still returns True when
the retry cannot complete.
---
Nitpick comments:
In `@src/plugin_system/plugin_loader.py`:
- Around line 238-276: The marker-writing logic in plugin_loader’s dependency
install flow is duplicated between the normal success path and the
uninstall-no-record-file retry fallback. Extract a small helper such as
_write_dependency_marker in PluginLoader to handle writing current_hash to
marker_file and calling ensure_file_permissions, then replace both existing
blocks with calls to that helper so marker behavior stays consistent in both
paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bcc11a60-c656-4c30-a715-45fcd78f90fe
📒 Files selected for processing (2)
src/plugin_system/plugin_loader.pytest/test_plugin_loader.py
…te logic CodeRabbit review caught a real inconsistency: if the --ignore-installed retry itself timed out, subprocess.TimeoutExpired propagated to the outer handler and returned False, failing plugin load — contradicting the intended "tolerate this specific apt/pip conflict" behavior, where a mere non-zero retry return code already returns True. Wraps the retry in its own try/except so a timeout is logged and tolerated the same way as any other retry failure. Also extracts the marker-writing logic (open/write/chmod, ignoring OSError) into _write_dependency_marker, since it was duplicated identically between the direct-success path and the apt-conflict-retry-fallback path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X1NnDduw53kTe67i5zWwYx
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
… alert CodeQL flagged _write_dependency_marker's open(marker_file, ...) as "uncontrolled data used in path expression" (high severity) once the marker-write logic was extracted into its own method. marker_file is actually safe — it's built from safe_plugin_dir, which install_dependencies sanitizes via os.path.basename() (CodeQL's own recognized py/path-injection sanitizer, per the existing comment a few lines above) — but CodeQL's interprocedural analysis doesn't carry that sanitized status across the new method boundary, since the sanitizer call and the open() sink were no longer in the same function. This exact code produced zero CodeQL findings before the extraction (in two duplicated inline blocks) and is unchanged in what data reaches it — only its location moved. Reverting the extraction (keeping CodeRabbit's other, independent timeout-handling fix) restores the previously-clean shape rather than trying to convince the analyzer's cross-function taint tracking that a refactor changed nothing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X1NnDduw53kTe67i5zWwYx
3 participants
Summary
Follow-up to #385. While fixing the Plugin Store's dependency install path there, I found one more gap in the same family of bugs, in
plugin_loader.py'sinstall_dependencies— the code path that runs on every plugin load, not just Store installs/updates.When pip fails with
uninstall-no-record-file(an apt/dnf-managed package likepython3-requestshas no pip RECORD file, so pip refuses to upgrade it in place), this function didn't retry with--ignore-installedlikeinstall_dependencies_apt.pyandsafe_pip_install.shalready do. It just logged a warning, assumed the dependency was satisfied, and wrote the success marker anyway. So if a plugin pins a newer version of a system-managed package (e.g.requests>=2.33.0), the plugin would silently keep running against whatever older version apt shipped — while the marker file claimed the pinned requirement was met. Same failure class as the weather/astralissue in #385, just self-inflicted by the "soft fallback": it doesn't fail loudly, it succeeds quietly with the wrong version.Changes
src/plugin_system/plugin_loader.py: onuninstall-no-record-file, retry the samepip installonce with--ignore-installedbefore falling back to the prior "assume satisfied" behavior. If the retry succeeds, the pinned version actually gets installed (shadowing the system-managed copy). If the retry also fails, keeps the existing tolerant behavior (logs and returnsTrue) so plugin loading isn't blocked over an edge case.test/test_plugin_loader.py: added two tests — one confirming the retry happens with--ignore-installedand succeeds, one confirming the prior tolerant fallback still holds if the retry itself fails.Type of change
Related issues
Related to #385 and #380.
Test plan
pytest test/test_plugin_loader.py test/test_store_manager_caches.py— 51 passed, including 2 new tests)python3 -m py_compileon both changed filesDocumentation
Plugin compatibility
Checklist
Notes for reviewer
This only affects plugins whose
requirements.txtpins a newer version of a package the OS also ships (e.g.requests,PIL,numpy). For plugins with no such conflict, behavior is unchanged.Generated by Claude Code
Summary by CodeRabbit